-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix after_commit on: :destroy not being triggered #404
Conversation
lib/paranoia.rb
Outdated
when :destroy | ||
transaction_include_destroy? | ||
when :update | ||
!(transaction_record_state(:new_record) || transaction_include_destroy?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use a nvm, I had some of the boolean logic mixed up in my head...x && y
instead of a !(x || y)
?
Remade this to accommodate rails 5.1 |
@npezza93 @BenMorganIO any idea on when this will be merged? We could use the change quite well. |
@rmachielse ¯\_(ツ)_/¯, I haven't heard anything so I think it's up to @BenMorganIO. |
@BenMorganIO what are your thoughts about this? |
@BenMorganIO is there any chance this gets merged soon? |
682ca2a
to
4b23356
Compare
@BenMorganIO anything I can do to speed this up? |
Is there anyone else who can pick this up? @jhawthorn maybe? |
That'd be very helpful for me as well, would like to see it on upstream. 👍 |
While this probably is the best approach; it differs from how it behaves with Rails 4.2. There a "soft delete" would trigger the update callback, and the destroy callback is called only for |
any update on this? I'm running into this now and surprised it hasn't been addressed yet |
Any update ? |
4b23356
to
8d32d02
Compare
+1 |
@npezza93 thanks for initial PR ❤️ @BenMorganIO will you merge & release it if we'll rebase && fix travis? /cc @jhawthorn |
I believe this is has been fixed with a combo of the latest release and rails >= 5.2 |
Shoutout to @skorfmann on #228 for for most of this! Fixes #217
Let me know if there are any issues with this approach.